The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Claude finished @julienrbrt's task in 1m 54s —— View job

Code Review - Force Inclusion E2E Tests

I've completed a comprehensive code review of the force inclusion e2e tests. Overall, the implementation is solid with good test coverage. Below are my detailed findings:

✅ Strengths

Test Design & Coverage

• Excellent test structure with two comprehensive scenarios: sequencer-only and full-node sync
• Good use of helper functions to reduce code duplication (setupSequencerWithForceInclusion, submitForceInclusionTx, enableForceInclusionInGenesis)
• Appropriate timeout values (30s for sequencer, 40s for full node) accounting for DA epoch mechanics
• Clear test progression: normal tx → forced inclusion tx in both tests

Code Quality

• Follows Go idioms and project conventions consistently
• Proper use of t.Helper() in all helper functions
• Good error messages in assertions (e.g., "Normal transaction not included")
• Appropriate use of require.Eventually for async validation
• Clean separation of concerns between setup and test execution

Documentation

• Clear inline comments explaining key concepts (e.g., DA epoch timing on line 163-164)
• Function documentation is clear and concise

⚠️ Issues & Recommendations

1. Configuration Validation Mismatch (High Priority)

Location: pkg/config/config.go:281-287

The tests use --evnode.da.forced_inclusion_namespace="forced-inc" but the configuration validation currently blocks this:

if len(c.DA.GetForcedInclusionNamespace()) > 0 {
    return fmt.Errorf("forced inclusion is not yet live")
}

Impact: These tests will fail when run outside the e2e test environment if config validation is enforced. The most recent commit (11d25fe) re-disabled force inclusion, but the tests were added assuming it's enabled.

Recommendation:

• Either update the config validation to allow force inclusion namespaces
• Or add a build tag / environment variable to enable force inclusion for testing
• The commented-out validation code should be uncommented when the feature goes live

Coverage Impact: The codecov report shows 2 missing lines in pkg/config/config.go - these are likely the commented-out validation lines (282-283).

2. HTTP Client Timeout Missing (Medium Priority)

Location: test/e2e/evm_force_inclusion_e2e_test.go:57

resp, err := http.Post(fiUrl, "application/json", bytes.NewBuffer(jsonData))

Issue: The HTTP POST uses http.DefaultClient which has no timeout, potentially causing tests to hang indefinitely if the force inclusion server is unresponsive.

Recommendation:

client := &http.Client{Timeout: 10 * time.Second}
resp, err := client.Post(fiUrl, "application/json", bytes.NewBuffer(jsonData))

3. File Permission Too Permissive (Low Priority - Security)

Location: test/e2e/evm_force_inclusion_e2e_test.go:40

err = os.WriteFile(genesisPath, newData, 0644)

Issue: Genesis files don't need to be group-readable. While this is just a test environment, following least-privilege principles is a good habit.

Recommendation: Use 0600 to match the pattern used for passphrase files (line 207 in evm_test_common.go).

4. Test Helper Return Value Inconsistency (Low Priority - Maintainability)

Location: test/e2e/evm_force_inclusion_e2e_test.go:71

func setupSequencerWithForceInclusion(t *testing.T, sut *SystemUnderTest, nodeHome string, fiPort int) (string, string)

Issue: The function returns (genesisHash, seqEthURL) but doesn't return the force inclusion URL, even though:

• It has access to fiAddr (line 96)
• Both test functions reconstruct fiUrl locally (lines 129, 181)
• This creates redundancy and potential for mismatch

Note: I see from the review comments that @julienrbrt initially requested this be changed, then clarified "Actually nvm. It is good like this." This is acceptable, but for future maintainability, consider whether returning all three values would be clearer.

5. Error Response Handling Could Be More Robust (Low Priority)

Location: test/e2e/evm_force_inclusion_e2e_test.go:66

require.Nil(t, res["error"], "RPC returned error: %v", res["error"])

Issue: If res["error"] exists but is a complex nested structure, the error message may not be fully informative.

Recommendation: Consider extracting and formatting the error more explicitly:

if errVal := res["error"]; errVal != nil {
    t.Fatalf("RPC returned error: %+v", errVal)
}
require.NotNil(t, res["result"], "RPC result is nil")

6. Magic Number - Epoch Value (Very Low Priority - Maintainability)

Location: test/e2e/evm_force_inclusion_e2e_test.go:93, 200

enableForceInclusionInGenesis(t, nodeHome, 2)

Issue: The epoch value 2 is a magic number that appears twice.

Recommendation: Consider extracting to a constant:

const TestForcedInclusionEpoch = 2 // Fast testing: force inclusion every 2 DA blocks

🔍 Additional Observations

Positive Patterns:

• Good use of descriptive variable names (fiUrl, fiAddr, fiPort)
• Appropriate logging at key test stages
• Consistent error handling throughout
• Reuse of existing e2e infrastructure (setupCommonEVMTest, setupFullNode)

Test Reliability:

• Timeouts are generous enough for CI environments
• No race conditions detected in concurrent operations
• Proper cleanup with defer client.Close()

Performance:

• Tests could take 30-40 seconds each due to DA epoch timing
• This is acceptable for e2e tests but consider marking with appropriate timeout annotations

📋 Summary

Critical Issues: 1 (Configuration validation mismatch)
Medium Issues: 1 (HTTP timeout)
Low Priority: 4 (File permissions, return value consistency, error handling, magic number)

The tests are well-written and follow project conventions. The main concern is the configuration validation that was re-disabled in the latest commit. This needs to be addressed before the tests can run in production:

• Option A: Enable force inclusion feature (remove the error return on line 285)
• Option B: Add conditional validation based on build tag/env var
• Option C: Update tests to not use force inclusion namespace until feature is ready

Given that the PR closes issue #2906 and has been approved, this appears to be intentionally adding tests in preparation for the feature launch.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.83%. Comparing base (aaae087) to head (11d25fe).
⚠️ Report is 3 commits behind head on main.

@@            Coverage Diff             @@
##             main    #2964      +/-   ##
==========================================
- Coverage   57.88%   57.83%   -0.06%     
==========================================
  Files          97       97              
  Lines        9306     9303       -3     
==========================================
- Hits         5387     5380       -7     
- Misses       3315     3319       +4     
  Partials      604      604              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

In our standup we decided not to enable the feature yet (11d25fe) but do so after #2965 completion.